test(search): close the band adoption gate's route-coverage holes - #1394
Conversation
The gate advertised "every production search route" and skipped the root dashboard page, so `/?mode=prescribing` and href-less Documents were never checked. Resolving those to `src/app/(search-app)/page.tsx` surfaced two more holes in the same gate that the original finding did not name, and fixing only the first would have produced a false orphan rather than coverage: - The walk was hard-capped at two import hops. The root route's real chain is four, so a fixed hop count silently under-reported reachability instead of failing loudly. - It followed neither `layout.tsx` nor `dynamic(() => import(...))`. Both matter here: `(search-app)/page.tsx` renders only a pass-through and its band arrives through the group layout's shared shell, while the dashboard code-splits its mode workspaces through `clinical-dashboard-lazy.tsx`. Replaced the hop-counted walk with a bounded BFS that resolves `@/` and relative specifiers, follows static and lazy imports, and treats a route's layouts as part of its rendered output — which is what App Router semantics actually mean. Each part was verified load-bearing rather than assumed: restoring the `pathOnly === "/"` early return drops the root route from the inventory, and capping depth back at 2 reports it as an orphan. The negative fixture now uses real files on disk and asserts both directions, since one that only ever returns false would pass against a walker that is broken outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 23 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR replaces heuristic band-adoption detection with file-based route and import traversal, adds coverage for dashboard and lazy-import paths, and updates the outstanding-issues ledger with revised findings and recommendation numbering. ChangesBand adoption reachability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1ebfcf90b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex caught a hole I introduced one commit earlier, and it was worse than the gap I was closing. Adding the route's layouts as reachability roots applied to every route, and the (search-app) layout transitively imports ClinicalDashboard — so any page under that group passed regardless of what it rendered. Reproduced by reducing services/page.tsx to <div />: the gate still passed 5/5. Scope layouts to dashboard-owned routes using the repo's own statement of ownership, isAlwaysStandaloneShellPath. Namespaced routes must now reach the band through their own page; only the root dashboard route, whose page renders a pass-through and whose band arrives via the shared shell, keeps its layouts. Re-verified both directions: with services/page.tsx gutted the gate now reports it as an orphan, and passes when restored. Added a test pinning reachabilityRoots directly, so the ownership scoping cannot be widened back without failing — a unit assertion rather than a fixture page, which would have created a real production route with no inbound link. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Re: "Stop counting unreachable layout imports as band adoption" — correct, and it was a hole I introduced. Fixed in I reproduced it exactly as described rather than reasoning about it. Reducing The gate passed with the services results page rendering nothing. Because I added every route's layouts as reachability roots, and Fix: scope layouts to dashboard-owned routes using the repo's own statement of ownership, Re-verified both directions:
On your fixture suggestion: I pinned Verification: Worth noting for the record: this is the second time on this PR that the reported symptom was the smaller part of the problem. The root-path bug led to the two-hop cap and the missing layout/dynamic handling; fixing those introduced this. A reachability model for a code-split App Router surface has more edges than it looks. Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f46d7a4e44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dered Codex is right that the walker reports adoption for a route that keeps its results import but stops rendering it; reducing services/page.tsx to <div /> with imports retained keeps the gate green. Recorded rather than fixed here, for two reasons. It is pre-existing — the previous two-hop walker had no render check either — and it is not a live risk: the same edit fails npm run lint with five no-unused-vars warnings under --max-warnings 0, so the composite static gate catches exactly the constructed scenario. The genuinely uncaught shape needs an import referenced somewhere non-rendering, which is deliberate rather than a slip. Tightening it properly means tracking which bindings appear in JSX element position or as the default export, since a re-export mounts with no JSX at all. That is a third expansion of this walker in one PR, and the second one introduced a worse hole than it closed, so it belongs in its own change with its own review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
Re: "Verify imported components are actually rendered" — accurate, and I'm deliberately not fixing it in this PR. Recorded as I reproduced it rather than reasoning about it. Keeping every import in So the walker does treat a discovered module as rendered. My earlier gut-check replaced the whole file including its imports, which is why it failed and this doesn't. Two things change the disposition, though. It is pre-existing, not introduced here. The previous two-hop walker matched The composite static gate already catches the constructed scenario. The same edit fails The repo runs Why not fix it now. Tightening it properly means tracking which imported bindings appear in JSX element position or as the default export — note One note on your suggested fixture: Verification on Generated by Claude Code |
|
@cursoragent resolve merge conflicts |
|
Resolved and pushed on tip ClassificationOne real conflict — simple. Only
No product-code conflicts. No conflicting intents beyond who owns Resolution
Complicated conflictsNone.   |
|
@Cursorsgent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers. |
|
@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers. |
|
CI/review closeout for #1394 is done on tip Root causes
Changes made
Validation
Remaining blockers
|
|
@cursoragent Inspect the PR and repository state, identify the root cause of every failing CI check, unresolved review comment, merge conflict, and related defect. Apply the smallest robust fix using existing project patterns and best practices, without unrelated changes. Run Bugbot and all relevant lint, type-check, test, build, and CI-equivalent commands, fix any issues found, then resolve only the review comments that are fully addressed. Summarise the root causes, changes made, validation results, and any remaining blockers. |
|
PR #1394 is merged as squash Root causes
Changes this pass
Validation
Remaining
|
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
isAlwaysStandaloneShellPath missed /tools (standalone mode home, but outside the Suspense always-standalone list), so gutting tools/page.tsx still reached the band via layout → ClinicalDashboard. Also clear the contradictory "Still live on main" clause in #96 now that the root-path gap is closed on this PR. Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Co-authored-by: BigSimmo <BigSimmo@users.noreply.github.com>
Prettier only, a single line. docs/outstanding-issues.md was left unformatted by the #96 wording edit on the previous tip, failing both ci/circleci: verify and Static PR checks — the third time this exact break has landed today. Recurs because format:check is not part of npm run test, typecheck, or lint, so a change can report a clean suite and a clean prettier run on the file it was focused on while the repository-wide check is red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
|
@codex review PR |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
* issues: archive five completed rows, delete none Triage pass over all 58 open items found five rows whose work is finished and whose next action is empty. Each moves from Open items to Resolved / archive with its fix evidence and the 2026-07-30 date: - #95 the pr-required aggregate now routes a cancelled result through a shared cancelled_error helper; guarded by seven cases that execute the extracted script, three mutation-proven. The red is deliberately retained, since GitHub counts a skipped required check as passing. - #96 every PR #1316 sub-item is dispositioned: the adoption-gate root-path gap closed on PR #1394, four findings were fixed independently, and the Therapy Compass retry-waiter finding was corrected to not-a-live-defect. - #104 a correction row with no next action - the worker's triple image read is an accepted peak-memory trade-off documented at worker/main.ts:866-869, not debt. Archived so a fourth audit does not re-file it. - #109 the branch sweep refuses on a shallow clone, an indeterminate result is its own failure, and the guard moved into the exported collector so the evidence-pack path fails closed too. - #115 the band adoption gate was redesigned to walk a real reachability graph rather than asking whether a file mentions the band. Nothing is deleted. The ledger contract is archive-only (SKILL.md:44 "Archive, never delete"; this file's own conventions: "rows are archived, not deleted, so the history stays auditable"), so no row qualifies for deletion. Row total is unchanged at 120: 58 -> 53 open, 62 -> 67 archived. Prettier widened the archive Outcome column to fit the new evidence, which repads the other archive rows; git diff --ignore-all-space is 7 insertions / 7 deletions, i.e. the five moved rows plus both separators. Verified: node scripts/check-outstanding-issues.mjs --self-test && node scripts/check-outstanding-issues.mjs -> "Outstanding-issues guard passed: 120 rows (53 open, 67 archived), unique ids, next-id=126 above the highest". npx prettier --check . -> "All matched files use Prettier code style!" Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011YdPS2KhKqz2buzsUgmX3c * docs: record PR 1428 review * docs: align issue 109 resolution date --------- Co-authored-by: Claude <noreply@anthropic.com>
3 participants
Summary
Closes the last live item from the PR #1316 review (
#096). Two files:tests/search-results-band-adoption.test.tsand the ledger row. No production code changes — this is the gate that guards the band, not the band itself.The reported gap was that
modeHrefToPagePathreturnednullforpathOnly === "/", so/?mode=prescribingand href-less Documents never entered the route inventory andsrc/app/(search-app)/page.tsxwas never checked. The gate claimed to cover "every production search route" and did not.Fixing only that would have made the gate red, not correct. Adding the root route surfaced two further holes in the same walker:
layout.tsx → shared-search-app-shell → global-search-shell → ClinicalDashboard → document-search-results. A fixed hop count silently under-reports reachability rather than failing loudly.layout.tsxnordynamic(() => import(...)). Both are load-bearing here.(search-app)/page.tsxrenders only a pass-through (HomePageClientreturnschildren ?? null) and its band arrives through the group layout's shared shell — so ignoring layouts reports a correctly-wired route as an orphan. And the dashboard code-splits its mode workspaces throughclinical-dashboard-lazy.tsx, so a static-only walk cannot see the band behind Differentials, Favourites or the prescribing workspace.Replaced the hop-counted walk with a bounded BFS that resolves
@/and relative specifiers, follows static and lazy imports, and treats a route's layouts as part of its rendered output — which is what App Router semantics actually mean.Verification
npm run test— 432 files, 4450 passed, 4 skippednpm run typecheck,npm run lint— exit 0npm run format:check— All matched files use Prettier code style!npm run docs:check-links— docs link check passed: 1365 repo path references resolve.npm run verify:ui— UI verification not run: no UI, routing, styling, reduced-motion or forced-colors behaviour changed. This touches one test file and one markdown file.npm run eval:retrieval:quality— not applicable; no retrieval, ranking, selection, chunking or scoring behaviour changed.npm run check:production-readiness— not applicable; no clinical workflow, privacy, environment, Supabase, source-governance or deployment behaviour changed.Each part of the fix was verified load-bearing rather than assumed:
pathOnly === "/"early returnMode-href discovery must include the root dashboard route…: expected false to be trueMAX_IMPORT_DEPTHback to2expected [ 'src/app/(search-app)/page.tsx' ] to deeply equal []The negative fixture now writes real files to a temp dir and asserts both directions — a route that reaches the band and one that doesn't. The previous in-memory fixture only ever asserted
false, which would pass against a walker broken outright.RAG impact: no retrieval behaviour change — this modifies a structural test and a documentation ledger row. No file under
src/lib/rag/**, clinical-search, retrieval-selection, released-search-order, ranking-config, the eval harness, the golden fixture, or the retrieval RPCs is touched.Risk and rollout
Clinical Governance Preflight
Not applicable — no ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output behaviour is touched. The change makes a structural test cover routes it previously skipped.
Notes
Worth recording why this took more than the one-line fix the finding implied: the gate's original premise — follow static imports from a page file to find the band — cannot hold for the dashboard-served modes, because the dashboard deliberately code-splits them and the root page delegates its chrome to a layout. The two-hop limit and the missing layout/dynamic handling were hiding that; the reported root-path bug was the symptom that led to them.
#096is updated to record the closure and the two additional defects, so the next reader knows the gate's reachability model changed and why.🤖 Generated with Claude Code
https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY
Generated by Claude Code
Summary by CodeRabbit
Tests
Documentation